Conversation
When max_queue_pairs is set to VHOST_MAX_QUEUE_PAIRS (128), VDUSE
calculates total_queues as max_queue_pairs * 2 + 1 = 257 to account
for the control queue. However, the virtqueue array was sized as
VHOST_MAX_QUEUE_PAIRS * 2, causing an out-of-bounds array access.
Fix by defining VHOST_MAX_VRING to explicitly account for the control
queue (VHOST_MAX_QUEUE_PAIRS * 2 + 1) and using it for the virtqueue
array size.
Fixes: f0a37cc6a1e2 ("vhost: add multiqueue support to VDUSE")
Cc: stable@dpdk.org
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
The virtio_net_ctrl_pop() function traverses descriptor chains from guest-controlled memory without validating that the descriptor index stays within bounds and without a counter to prevent infinite loops from circular chains. A malicious guest could craft descriptors with a next field pointing out of bounds causing memory corruption, or create circular descriptor chains causing an infinite loop and denial of service. Add bounds checking and a loop counter to both descriptor chain traversal loops, similar to the existing protection in virtio_net.c fill_vec_buf_split(). Fixes: 474f4d7 ("vhost: add control virtqueue") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
The mmap() function returns MAP_FAILED on failure, not NULL. The current check for !mmap_addr will never detect mmap failures. When mmap fails but the error is not detected, an invalid address (-1) is inserted into the IOTLB cache via vhost_user_iotlb_cache_insert(). Subsequent attempts to access this address will cause memory corruption or crash. Fix by checking for MAP_FAILED instead of NULL. Also add strerror to the error message for easier debugging. Fixes: f27d520 ("vhost: add VDUSE callback for IOTLB miss") Cc: stable@dpdk.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThis PR fixes VDUSE-related issues in the vhost library by hardening virtio control queue descriptor walking, correcting error handling for IOTLB mmaps, and aligning maximum vring counts and virtqueue array sizing with the supported number of queue pairs plus the control queue. Updated class diagram for vhost vring and virtqueue sizingclassDiagram
class VHOST_MAX_QUEUE_PAIRS {
<<macro>>
+0x80
}
class VHOST_MAX_VRING {
<<macro>>
+VHOST_MAX_QUEUE_PAIRS * 2 + 1
+Comment: 2 vrings per queue pair plus 1 control queue
}
class vhost_virtqueue {
<<struct>>
+... // fields not changed in this PR
}
class virtio_net {
<<struct>>
+int extbuf
+int linearbuf
+vhost_virtqueue* virtqueue[VHOST_MAX_VRING]
+rte_rwlock_t iotlb_pending_lock
+vhost_iotlb_entry* iotlb_pool
}
VHOST_MAX_QUEUE_PAIRS <|-- VHOST_MAX_VRING : defines
VHOST_MAX_VRING <.. virtio_net : array_size
vhost_virtqueue <.. virtio_net : contains_array_of
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR improves VHOST backend robustness by refining IOTLB mmap error detection, redefining the maximum vring count macro based on queue pair configuration, and adding descriptor chain validation with bounds checking and safety counters to prevent invalid descriptor traversal. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new descriptor-chain guard logic in virtio_net_ctrl_pop() uses uint16_t for nr_descs and cnt while nr_descs is derived from a 64-bit desc_len; consider using a wider type (e.g., uint32_t/size_t) to avoid truncation and ensure the bounds checks are effective for larger descriptor tables.
- The descriptor-chain validation pattern (nr_descs/cnt checks and error logging) is now duplicated in two places in virtio_net_ctrl.c; consider factoring this into a small helper to keep the logic consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new descriptor-chain guard logic in virtio_net_ctrl_pop() uses uint16_t for nr_descs and cnt while nr_descs is derived from a 64-bit desc_len; consider using a wider type (e.g., uint32_t/size_t) to avoid truncation and ensure the bounds checks are effective for larger descriptor tables.
- The descriptor-chain validation pattern (nr_descs/cnt checks and error logging) is now duplicated in two places in virtio_net_ctrl.c; consider factoring this into a small helper to keep the logic consistent and easier to maintain.
## Individual Comments
### Comment 1
<location> `lib/vhost/virtio_net_ctrl.c:60-63` </location>
<code_context>
goto err;
}
+ nr_descs = desc_len / sizeof(struct vring_desc);
desc_idx = 0;
} else {
</code_context>
<issue_to_address>
**suggestion:** Validate that the indirect descriptor length is a non-zero multiple of sizeof(struct vring_desc) before computing nr_descs.
Per the virtio spec, indirect descriptor tables must be non-zero and aligned to `sizeof(struct vring_desc)`. Relying on integer truncation means malformed tables (zero length or not a multiple) collapse into `nr_descs == 0` and are only caught later as a generic invalid chain. Instead, consider explicitly checking `desc_len == 0 || desc_len % sizeof(struct vring_desc) != 0` and failing early with a specific error log for malformed indirect tables.
```suggestion
}
/* Validate indirect descriptor table length before using it */
if (unlikely(desc_len == 0 ||
desc_len % sizeof(struct vring_desc) != 0)) {
VHOST_CONFIG_LOG(dev->ifname, ERR,
"Malformed indirect descriptor table");
goto err;
}
nr_descs = desc_len / sizeof(struct vring_desc);
desc_idx = 0;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36991"
Summary by Sourcery
Harden vhost VDUSE and control virtqueue handling and align vring limits with the maximum queue pair configuration.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.